Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[new-parser] Allow package to be used as an identifier #5819

Closed
wants to merge 1 commit into from

Conversation

yurloc
Copy link
Contributor

@yurloc yurloc commented Apr 2, 2024

Fixes

DRAFT

Let's discuss whether this is a good approach.

How to replicate CI configuration locally?

Build Chain tool does "simple" maven build(s), the builds are just Maven commands, but because the repositories relates and depends on each other and any change in API or class method could affect several of those repositories there is a need to use build-chain tool to handle cross repository builds and be sure that we always use latest version of the code for each repository.

build-chain tool is a build tool which can be used on command line locally or in Github Actions workflow(s), in case you need to change multiple repositories and send multiple dependent pull requests related with a change you can easily reproduce the same build by executing it on Github hosted environment or locally in your development environment. See local execution details to get more information about it.

How to retest this PR or trigger a specific build:
  • for pull request and downstream checks

    • Push a new commit to the PR. An empty commit would be enough.
  • for a full downstream build

    • for github actions job: add the label run_fdb
  • for Jenkins PR check only

@@ -198,6 +198,7 @@ drlIdentifier
| RECORD
| VAR
| THIS
| PACKAGE
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tkobayas @mariofusco @gitgabrio Together with THIS on the line above, this is just the tip of the iceberg. If we want to keep the new parser 100% backward compatible, we need to A) also add all the other Java keywords, for example, long, break, for, etc. or B) revert closer to the old approach and stop using JavaLexer that defines all these keyword tokens.

B) means using just a few tokens for operators, parentheses, and literals of various types, and IDENTIFIER for everything else on the Lexer level. The parser then needs to use semantic predicates (e.g. helper.validateIdentifierKey(DroolsSoftKeywords.PACKAGE) to see whether the next IDENTIFIER token actually contains package).

A) means listing all JavaLexer tokens for various Java keywords here and also replacing nearly all usages of IDENTIFIER with drlIdentifier.

Copy link
Contributor

@tkobayas tkobayas Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the original test failure #5818 org.kie.declarativetypes.JavaBeansEventRoleTest#testImportBean is a very exceptional case.

        String drl = "package some.other.package;\n" +
                "import org.drools.compiler.Bean;\n" +
                "declare Bean\n" +
                "  @role(event)\n" +
                "end";

The test doesn't fail with the old parser, because the package some.other.package doesn't have anything to compile actually. If a user adds a rule to the package, it would fail on the java compilation phase even with the old parser.

DRL is more or less based on Java syntax, so I think, theoretically, it's fine to fail such a java keyword usage (which normally fails with java compiler) at the parse phase with the new parser (As written in #5818, no-error-report is another issue, though). In other words, fix the test case not to use package in package.

C) Keep excluding java keywords from drlIdentifier

It's against the policy "100% backward compatible with the old parser", but sounds acceptable. WDYT?

(We may face a situation that we will need to choose option A or B in the future, though)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @yurloc @tkobayas @mariofusco
sorry I could provide only very general observations:

  1. I would stick with 100% backward compatibility; without that, something that was working previously, simply stop to work, and it is not clear who/how would fix the issues that will popup, nor how much time this could take
  2. I wonder (again, just asking) if there is not a programmatic way to automatically add all the java keywords from JavaLexer to drlIdentifier : drlKeywords - and then keep going with option A (IIUC)

Wdyt ? Am I missing something ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I would stick with 100% backward compatibility; without that, something that was working previously, simply stop to work, and it is not clear who/how would fix the issues that will popup, nor how much time this could take

Okay, I'm not opinionated on this. We can go with Option A and leave Option C as a future enhancement possibility.

  1. I wonder (again, just asking) if there is not a programmatic way to automatically add all the java keywords from JavaLexer to drlIdentifier : drlKeywords - and then keep going with option A (IIUC)

There is no programmatic way because those Java keywords are individually declared in JavaLexer.g4. I think explicitly listing them in drlIdentifier is readable enough.

Thanks,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok @tkobayas
Anyway, just remember that copying/duplicating stuff creates problem of maintanability, not readability.
Anyway, fine for me 👍

@yurloc
Copy link
Contributor Author

yurloc commented May 17, 2024

Superseded by #5958.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants